Skip to content

Conversation

@mstruebing
Copy link
Member

@mstruebing mstruebing commented Jan 17, 2026

fixes #2746

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 17, 2026
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 17, 2026
Comment on lines 9 to 12
const namespace = process.argv[2] || 'default';
const deploymentName = process.argv[3] || 'demo-deployment';
const localPort = parseInt(process.argv[4] || '8080', 10);
const remotePort = parseInt(process.argv[5] || '8080', 10);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const namespace = process.argv[2] || 'default';
const deploymentName = process.argv[3] || 'demo-deployment';
const localPort = parseInt(process.argv[4] || '8080', 10);
const remotePort = parseInt(process.argv[5] || '8080', 10);
const namespace = process.argv[2] ?? 'default';
const deploymentName = process.argv[3] ?? 'demo-deployment';
const localPort = parseInt(process.argv[4] ?? '8080', 10);
const remotePort = parseInt(process.argv[5] ?? '8080', 10);


// This creates a local server that forwards traffic to a deployment in Kubernetes
// by resolving the deployment to its first ready pod and port-forwarding to that pod.
// Usage: npx ts-node port-forward-deployment.ts [namespace] [deploymentName] [localPort] [remotePort]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should promote using ts-node. I believe it's unmaintained at this point. There is tsx, but also Node can run TypeScript directly now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Today I learned! Thank you I didn't knew NodeJS is capable of running typescript direclty.
In case anyone else stumbles upon this comment: https://nodejs.org/en/learn/typescript/run-natively


const forward = new k8s.PortForward(kc);

const namespace = process.argv[2] || 'default';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment on these lines about using ?? instead of ||.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't necessarily disagree, but why ?? instead of ||?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in general we should prefer ?? unless we explicitly want to default on all falsy values. In this case, process.argv[x] will be undefined if not set, so they should behave identically. I haven't benchmarked it, but I'd also expect ?? to be faster since it does slightly less work under the hood.


// This creates a local server that forwards traffic to a service in Kubernetes
// by resolving the service to its first ready pod and port-forwarding to that pod.
// Usage: npx ts-node port-forward-service.ts [namespace] [serviceName] [localPort] [remotePort]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about ts-node here.

Comment on lines 9 to 12
const namespace = process.argv[2] || 'default';
const deploymentName = process.argv[3] || 'demo-deployment';
const localPort = parseInt(process.argv[4] || '8080', 10);
const remotePort = parseInt(process.argv[5] || '8080', 10);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?? here as well please.

Comment on lines 92 to 95
.catch((error) => {
console.error('Deployment port forward error:', error.message);
socket.destroy();
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use async-await and try...catch to be more consistent.

});

// Give the server a moment to start
await setTimeout(500);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? Once the listen() Promise above resolves, the server should be ready to serve traffic.

const serviceServer = net.createServer((socket) => {
portForward
.portForwardService(namespace, serviceName, [containerPort], socket, null, socket)
.catch((error) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here about using async-await and try...catch.

});

// Give the server a moment to start
await setTimeout(500);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question about this being necessary.

// Test connection to service via port-forward
for (let i = 0; i < 5; i++) {
try {
const response = await new Promise<string>((resolve, reject) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this logic all the same as above? If so, maybe it should be placed in its own function.

@mstruebing
Copy link
Member Author

@cjihrig thanks for your review. I think I've addressed all your comments. I've made them in separate commits to easier keep track of what I already did and what I still need to do so it also be easy to review.

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Maybe we should give it another day for other reviewers to weigh in, but I'm fine with landing this.

@@ -1,40 +1,40 @@
import * as k8s from '@kubernetes/client-node';
// import * as k8s from '@kubernetes/client-node';
import * as k8s from '../../../dist/index.js';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like maybe it was left over from local testing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed it, good catch.
Prettier autoformatted the file so I would leave it in here.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjihrig, mstruebing

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

assert.strictEqual(podsReady, true, 'Deployment pods did not become ready in time');

try {
// Test 1: Port forward to deployment
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
can we move test 1, test2, test3 into named functions to organize main test func body?

deploymentSuccessTest(...)
serviceSuccessTest(...)
serviceNoSelectorTest(...)

// Wait for pods to be ready
console.log('Waiting for deployment pods to be ready...');
let podsReady = false;
for (let i = 0; i < 60; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit the cycles/timouts are easier to understand when they are extracted into const

Suggested change
for (let i = 0; i < 60; i++) {
const MAX_POD_READY_CHECKS = 60;
const POD_READY_CHECK_POLL_DELAY_MS = 1000;
for (let i = 0; i < MAX_POD_READY_CHECKS; i++) {

console.log(`Deployment is ready with ${deployment.status?.readyReplicas} replicas`);
break;
}
await setTimeout(1000);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
await setTimeout(1000);
await setTimeout(POD_READY_CHECK_POLL_DELAY_MS);

}

async function testPortForwardConnection(testPort: number, label: string): Promise<boolean> {
for (let i = 0; i < 5; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit similar magic number or comment here

Copy link
Contributor

@davidgamero davidgamero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM just a couple nits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Port Forward currently only allows forwarding to pod resource type

5 participants